-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8240567: MethodTooLargeException thrown while creating a jlink image #10704
Conversation
👋 Welcome back koppor! A progress list of the required criteria for merging this PR into |
Webrevs
|
Hello Oliver, the GitHub actions job failures appear related to this change. For example:
|
This one has been on my list to fix for sometime, it's okay if you take it. Can you look at the tests in test/jdk/tools/jlink, that is where we will need to add the test for this. |
src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
Show resolved
Hide resolved
I looked and started to create some Java files at |
Hello Oliver,
Do you mean you are looking for inputs on how to run these tests locally? If so, then the testing guide https://github.com/openjdk/jdk/blob/master/doc/testing.md as well as the jtreg project documentation https://openjdk.org/jtreg/ might help. |
I think there are still issues. As a quick test, set the maximum number of descriptors to generate per method to less than 70, so that you get Sub0, Sub1, ... generated. Then try |
Looking at the existing code (without the proposed changes in this PR), it appears that it's not just the number of So would it be correct to say that even if this PR proposes to use a limit like 99 to split it into multiple methods, it's still possible that the method size limit could be hit with a much lower number of Is that something that should be considered/addressed as part of this change? |
That's right and we may have to do further work in the future to avoid generating methods larger than 64k. The approach taken in this PR is okay for now but the current patch needs a bit more work to generate correct code. |
Thank you for the hint - this is the output. -- Now, I have more food-for-thought. ^^ $ build/windows-x86_64-server-release/images/jdk/bin/java --add-modules ALL-MODULE-PATH -Xlog:init=debug -XX:+UnlockDiagnosticVMOptions -XX:+BytecodeVerificationLocal -version
Error occurred during initialization of boot layer
java.lang.VerifyError: Bad local variable type
Exception Details:
Location:
jdk/internal/module/SystemModules$all.moduleDescriptorsSub1([Ljava/lang/module/ModuleDescriptor;)V @19: aload
Reason:
Type top (current frame, locals[15]) is not assignable to reference type
Current Frame:
bci: @19
flags: { }
locals: { 'jdk/internal/module/Builder', '[Ljava/lang/module/ModuleDescriptor;' }
stack: { 'jdk/internal/module/Builder', 'jdk/internal/module/Builder', '[Ljava/lang/module/ModuleDescriptor$Requires;', '[Ljava/lang/module/ModuleDescriptor$Requires;', integer }
Bytecode:
0000000: bb00 1159 1309 ddb7 0016 4b2a 2a05 bd00
0000010: 1859 0319 0f12 13b8 0288 5359 0419 1213
0000020: 02a4 b802 8853 b600 1c2a 07bd 001e 5903
0000030: 1904 1309 dfb8 0038 5359 0419 0413 09e1
0000040: b800 3853 5905 1904 1309 e3b8 0038 5359
0000050: 0619 0413 09e5 b800 3853 b601 8d2a 03bd
0000060: 018f b601 932a 1917 b601 dd57 2a03 bd01
0000070: dfb6 01f4 1309 df13 09e1 1309 e313 09e5
0000080: b801 004d 2a2c b602 7357 2a13 0275 b602
0000090: 7957 2b10 462a 1309 e6b6 027e 53b1
at java.base/jdk.internal.module.SystemModulesMap.allSystemModules(Unknown Source)
at java.base/jdk.internal.module.SystemModuleFinders.allSystemModules(SystemModuleFinders.java:102)
at java.base/jdk.internal.module.ModuleBootstrap.boot2(ModuleBootstrap.java:236)
at java.base/jdk.internal.module.ModuleBootstrap.boot(ModuleBootstrap.java:174)
at java.base/java.lang.System.initPhase2(System.java:2214) |
@koppor This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Would it be possible to paste in a summary on the VerifyError with the previous iteration? If I read the latest update then the limit per helper method has been bump to avoid it, is that right? |
Isn't this #10704 (comment)?
Yes. Then, the compiler still works - and we can try to debug using the test case (yet to be finalized). |
Okay, so maybe the PR should be returned to draft until it is ready. |
@koppor Should we continue to just ignore this PR for now? The current patch is test only, I don't know if that is deliberate or not. |
Yes, we are currently trying to set up the test, so it results in the original method too large error. Then we will re add the module splitting. (Working on this together with @koppor ) |
|
@AlanBateman We now have a reproducible JTREG test case that throws the MethodTooLargeException. We will now readd the module splitting handling. |
We readded the splitting code and the test is passing. In the test we could make it work with up to 130 modules where each module n requires all modules from 0...n Any further optimization ideas? |
The patch looks similar to the original. As a quick test, I changed the threshold to 10 (from 90) to force the creation of helper methods and it fails as expected with:
Do you want to continue with this issue? |
@koppor This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@koppor this pull request can not be integrated into git checkout fix-8240567
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
a52dd37
to
3580702
Compare
Current state:
|
While it does not compile currently (there is an error in the SystemModulesPlugin generate m, I replaced SystemModulesPlugin with the master version and run the test again. It even worked with 500 modules. |
Okay, we we found now out it's still happening but at runtime. So we will try to get this working again |
…lder) Co-authored-by: Christoph Schwentker <siedlerkiller@gmail.com>
Still getting errors at runtime now: (I compiled JabRef with jpackage against a local build)
|
We close this PR as this takes longer time for "code and fix". |
Fix for JDK-8240567: "MethodTooLargeException thrown while creating a jlink image".
Java still has a 64kb limit: A method may not be longer than 64kb. The idea of the fix is to split up the generated methods in several smaller methods
Progress
Error
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/10704/head:pull/10704
$ git checkout pull/10704
Update a local copy of the PR:
$ git checkout pull/10704
$ git pull https://git.openjdk.org/jdk.git pull/10704/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10704
View PR using the GUI difftool:
$ git pr show -t 10704
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10704.diff
Webrev
Link to Webrev Comment